Conversation
…tore best practice guide
There was a problem hiding this comment.
Code Review
This pull request updates the Datastore gRPC channel pool configuration by reducing the maximum RPCs per channel and adding a minimum RPCs per channel setting. Unit tests were updated to match these defaults and to follow standard assertion argument ordering. A review comment identifies an inconsistency in the documentation where a comment still refers to the previous limits of 100 streams and 500 QPS, which should be addressed to ensure documentation accuracy.
| // Default to a slightly larger channel count to handle a larger initial QPS (up to ~500 QPS as | ||
| // each channel can handle 100 max streams). This default remains a bit conservative and will |
There was a problem hiding this comment.
The comment at lines 57-58 refers to a stale implementation (~500 QPS and 100 max streams) which is inconsistent with the updated configuration. According to repository rules, comments referring to stale implementations should be removed rather than updated.
References
- Remove comments that refer to stale implementations instead of updating them.
There was a problem hiding this comment.
Is this relevant? If INIT_CHANNEL_COUNT = 5 and CHANNEL_POOL_MAX_RPCS_PER_CHANNEL = 50, the max QPS would be 250, right?
There was a problem hiding this comment.
500 streams is the theoretical max as GFE will enforce a limit of 100 streams/ channel. However CHANNEL_POOL_MAX_RPCS_PER_CHANNEL refers to the channel pool config where we aim to have an avg of 50 streams/ channel
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the Datastore gRPC channel pool settings by adding a minimum RPCs per channel configuration and lowering the maximum RPCs per channel. It also updates documentation and unit tests. A review comment points out that a documentation block regarding QPS capacity is stale and should be removed rather than updated, following repository guidelines.
…tore best practice guide (googleapis#12919)
No description provided.